Skip to content

gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators#118454

Merged
DinoV merged 4 commits into
python:mainfrom
DinoV:nogil_type_crash
May 6, 2024
Merged

gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators#118454
DinoV merged 4 commits into
python:mainfrom
DinoV:nogil_type_crash

Conversation

@DinoV

@DinoV DinoV commented May 1, 2024

Copy link
Copy Markdown
Contributor

Lookups from the type cache can be exposed to some races. One of those is that _PyType_Lookup isn't increfing the result, so the resulting value can become invalid at any point. This adds a new _PyType_Fetch API which does the incref. Most places are updated to use the new API except for the specializer which isn't safe in free-threaded builds yet.

But there are also issues around when we do a store into the dict and signal that the type is modified. A race can occur where the old value is removed from the dict and before we invalidate the type version. This can be seen today w/ the GIL when the value in the dict has a finalizer - accessing the type cache and the type's mapping proxy return inconsistent results.

This fixes these issues by making the update to the dict and the type modified update an atomic operation. We also capture the previous value in the dictionary and don't free it until after we've made the atomic update. This basically inlines a much simplified version of _PyObject_GenericSetAttrWithDict into type_setattro which can lock mutation against types as well as against the type's dict.

When updating we first clear the type version so concurrent reads of the type cache won't hit. We then replace the existing value and finish the type modified process.

Makes setting an attribute on a class and signaling type modified atomic
Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions comments below. Biggest questions is that now that we are accessing the dict directly instead of through _PyObject_GenericSetAttrWithDict, how do we know that the other cases handled by _PyObject_GenericSetAttrWithDict are not necessary in _Py_type_getattro?

Also, I think this merits a NEWS entry.

Comment thread Include/cpython/object.h Outdated
Comment thread Objects/dictobject.c Outdated
Comment thread Objects/dictobject.c
Comment thread Objects/object.c Outdated
Comment thread Objects/typeobject.c Outdated
Comment thread Objects/typeobject.c
Comment thread Objects/typeobject.c
Comment thread Objects/typeobject.c Outdated
Comment thread Objects/typeobject.c
Comment thread Lib/test/test_free_threading/test_type.py
@DinoV

DinoV commented May 1, 2024

Copy link
Copy Markdown
Contributor Author

Some questions comments below. Biggest questions is that now that we are accessing the dict directly instead of through _PyObject_GenericSetAttrWithDict, how do we know that the other cases handled by _PyObject_GenericSetAttrWithDict are not necessary in _Py_type_getattro?

This is part of the reason why I assert:

    assert(!_PyType_HasFeature(metatype, Py_TPFLAGS_INLINE_VALUES));
    assert(!_PyType_HasFeature(metatype, Py_TPFLAGS_MANAGED_DICT));

In type_setattro. Type objects will always have a dictptr, so we'll either go to _PyObjectDict_SetItem if the dict doesn't exist yet or do the get/set if they do. _PyObjectDict_SetItem is just going to do a get/set as well. And because types don't have a managed dict they don't have shared keys.

Also, I think this merits a NEWS entry.

@DinoV DinoV marked this pull request as ready for review May 1, 2024 20:39
Fix some formatting
Add news blurb
Use PyDictObject * more
Rename _PyDict_GetItemRef_LockHeld to _PyDict_GetItemRef_Unicode_LockHeld
Reduce iterations in test
Expose _PyObject_SetAttributeErrorContext for attaching context
@DinoV

DinoV commented May 1, 2024

Copy link
Copy Markdown
Contributor Author

Also, I think this merits a NEWS entry.
I opened #118492 to track the issue that is fixed when the GIL is present and blurb'd it.

@DinoV DinoV force-pushed the nogil_type_crash branch from 705538b to 8074d0d Compare May 1, 2024 21:23
@colesbury colesbury changed the title [gh-118362] Fix thread safety around lookups from the type cache in the face of concurrent mutators gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators May 2, 2024
Comment thread Include/internal/pycore_object.h Outdated
extern PyTypeObject* _PyType_CalculateMetaclass(PyTypeObject *, PyObject *);
extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *);
extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int);
extern int _PyObject_SetAttributeErrorContext(PyObject* v, PyObject* name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extern int _PyObject_SetAttributeErrorContext(PyObject* v, PyObject* name);
extern int _PyObject_SetAttributeErrorContext(PyObject *v, PyObject *name);

Comment thread Objects/typeobject.c
PyObject *dict = lookup_tp_dict(_PyType_CAST(base));
assert(dict && PyDict_Check(dict));
res = _PyDict_GetItem_KnownHash(dict, name, hash);
if (_PyDict_GetItemRef_KnownHash((PyDictObject *)dict, name, hash, &res) < 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PyErr_Occurred() check below can be removed now.

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few minor issues:

  • We need to handle the PyDict_New() returning NULL case in type_setattro
  • Minor formatting in _PyObject_SetAttributeErrorContext
  • PyErr_Occurred() case is no longer need in find_name_in_mro

@rhettinger rhettinger removed their request for review May 3, 2024 23:18
@DinoV DinoV merged commit 5a1618a into python:main May 6, 2024
@bedevere-bot

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot iOS ARM64 Simulator 3.x has failed when building commit 5a1618a.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1380/builds/238) and take a look at the build logs.
  4. Check if the failure is related to this commit (5a1618a) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1380/builds/238

Failed tests:

  • test_free_threading

Failed subtests:

  • test_attr_cache - test.test_free_threading.test_type.TestType.test_attr_cache

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/test/test_free_threading/test_type.py", line 37, in test_attr_cache
    with Pool(NTHREADS) as pool:
         ~~~~^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/dummy/__init__.py", line 124, in Pool
    return ThreadPool(processes, initializer, initargs)
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/pool.py", line 930, in __init__
    Pool.__init__(self, processes, initializer, initargs)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/pool.py", line 196, in __init__
    self._change_notifier = self._ctx.SimpleQueue()
                            ~~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/context.py", line 113, in SimpleQueue
    return SimpleQueue(ctx=self.get_context())
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/queues.py", line 361, in __init__
    self._rlock = ctx.Lock()
                  ~~~~~~~~^^
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/context.py", line 67, in Lock
    from .synchronize import Lock
  File "/Users/buildbot/Library/Developer/XCTestDevices/2E72F7FC-E4ED-40AC-B022-F9C50E4B0A1E/data/Containers/Bundle/Application/4CB60694-FBAA-49BE-969E-073C108C659C/iOSTestbed.app/python/lib/python3.13/multiprocessing/synchronize.py", line 17, in <module>
    import _multiprocessing
ModuleNotFoundError: No module named '_multiprocessing'

class A:
attr = 1

@unittest.skipIf(is_wasi, "WASI has no threads.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, probably better to use @threading_helper.requires_working_threading()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS and Android do both have working threading, but they don't support subprocesses. In theory multiprocessing.dummy.Pool should work on these platforms, but in practice it doesn't because it imports too much of the main multiprocessing module.

The simplest solution is probably to use concurrent.futures.ThreadPoolExecutor instead.

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
… in the face of concurrent mutators (python#118454)

Add _PyType_LookupRef and use incref before setting attribute on type
Makes setting an attribute on a class and signaling type modified atomic
Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
@DinoV DinoV deleted the nogil_type_crash branch May 31, 2024 18:22
@filmor filmor mentioned this pull request Dec 7, 2025
3 tasks
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 7, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 8, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 8, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 8, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 8, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Dec 9, 2025
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
filmor added a commit to pythonnet/pythonnet that referenced this pull request Feb 24, 2026
Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454
jhonabreul pushed a commit to QuantConnect/pythonnet that referenced this pull request Jul 7, 2026
* Initial 3.14 commit

(cherry picked from commit caac33d)

* Apply alignment fix

(cherry picked from commit e10d333)

* Disable problematic GC tests

(cherry picked from commit e976558)

* Set ht_token to NULL in Python 3.14

(cherry picked from commit 65af098)

* Workaround for blocked PyObject_GenericSetAttr in metatypes

Python 3.14 introduced a new assertion that prevents us from using
PyObject_GenericSetAttr directly in our meta type. To work around
this, we manipulate the type dict directly.

This workaround is a simplified variant of Cython's workaround from
cython/cython#6325.

The relevant Python change is in
python/cpython#118454

(cherry picked from commit 08550d0)

* Use PyThreadState_GetUnchecked on Python 3.13

(cherry picked from commit f3face0)

* Remove deprecated function call

(cherry picked from commit 8dfe408)

* Assign True instead of None to __clear_reentry_guard__

Not at all sure why this helps, but when assigning `None` instead, the
object is gone at the time of garbage collection.

(cherry picked from commit 8e0333d)

* Move tp_clear workaround to .NET

In Python 3.14, the objects __dict__ seems to already be half
deconstructed, leading to crashes during garbage collection.

Since gc in Python is single-threaded (I think :)), it should
be fine to have a single static for this. If that is not true,
we can always use a thread-local instead.

(cherry picked from commit 908e13b)

* Use non-BOM encodings (pythonnet#2370)

* Use non-BOM encodings

The documentation of the used `PyUnicode_DecodeUTF16` states that not passing `*byteorder` or passing a 0 results in the first two bytes, if
they are the BOM (U+FEFF, zero-width no-break space), to be interpreted and skipped, which is incorrect when we convert a known "non BOM" string, which all strings from C# are.

(cherry picked from commit 195cde6)

* Preserve SyntaxError source line in message on Python 3.12+

Python 3.12 eagerly normalizes the error indicator, so PyErr_Fetch now
hands us the SyntaxError instance (whose str() omits the offending source
line) instead of the raw args tuple (whose str() included it). Callers that
surface PythonException.Message for compile diagnostics therefore lost the
offending source text on 3.12+.

GetMessage now re-appends the SyntaxError 'text' attribute when present.
This is a no-op on <=3.11 (there the fetched value is a tuple without the
SyntaxError attributes) and only affects SyntaxError messages.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Make embed tests compatible with Python 3.12+ behavior changes

Three CPython behavior changes surfaced as failures/crashes once the
overload-resolution crash was fixed, all on 3.12+:

- ClassManagerTests.BindsCorrectOverloadForClassName crashed the host with
  "Python memory allocator called without holding the GIL". TestClass2's
  Get(PyObject o) re-enters Python via ToPython() while MethodBinder has
  released the GIL (allow_threads) around the managed call. A managed
  callback that re-enters Python must re-acquire the GIL; tolerated on
  <=3.11, fatal on 3.12+. Wrap the body in using (Py.GIL()).

- TestGetsPythonCodeInfoInStackTrace[ForNestedInterop]: 3.12+ adds caret
  indicator lines (e.g. "~~~^^^") under source lines in tracebacks, shifting
  the positional assertions. Drop caret-only lines before asserting
  (no-op on <=3.11).

- Codecs.ExceptionDecodedNoInstance: 3.12 eagerly normalizes exceptions, so
  the error indicator always carries an instance ("value"); the instanceless
  scenario this decoder targets can no longer be produced. Guard the test to
  <3.12.

Verified: full embed suite green on 3.11 (910/910) with these changes; the
three previously-failing tests pass on 3.14.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Add Python 3.12 / 3.13 ABI offsets and CI jobs

The fork resolves PyTypeObject field offsets from the hardcoded
TypeOffset{major}{minor} tables (it does not run geninterop at build),
so a missing table makes ABI.Initialize throw "Python ABI v... is not
supported" and every test on that version fails at PythonEngine init.
Only 3.6-3.11 and 3.14 tables were present.

Vendor the 3.12 and 3.13 tables from pythonnet/pythonnet upstream
(byte-identical to upstream master; same source as the already-present
TypeOffset314) and add 3.12 + 3.13 to the CI matrix.

Local verification (uv standalone CPython 3.13, this branch's fixes):
embed suite ABI-initializes correctly and runs 847 passed / 0 failed
(parity with 3.14). 3.12 table is vendored from the same authoritative
source; CI exercises it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Support Python 3.13/3.14 re-init in tests; drop obsolete TestDomainReload

The embed-test suite re-initializes the interpreter per fixture. On CPython
3.13/3.14 the suite aborted with "Failed to import encodings module" - not a
filesystem problem (strace shows the file opens fine) but interpreter import
state corrupted across re-initialization.

Root cause isolated to a single test: TestPythonEngineProperties.SetPythonPath.
It uses PythonEngine.PythonPath, which pins a fixed module search path via the
deprecated Py_SetPath. CPython 3.13+ keeps that path config in _PyRuntime across
Py_Finalize and offers no way to reset it back to auto-computation without the
PyConfig API, so once this test runs every later re-initialization in the same
process is forced onto the pinned path and eventually cannot bootstrap encodings.
All other fixtures - including the normal Initialize/Shutdown cycles in
pyinitialize and TestFinalizer - run fine in a single process.

Run only SetPythonPath in its own test process so it cannot pollute the rest of
the suite. Verified locally: full embed suite green on 3.11, 3.13 and 3.14
(main run 908 / SetPythonPath 1, 0 failures); no regression.

Also delete TestDomainReload: AppDomain reload is not supported on modern .NET
(single-domain), so those tests (MarshalByRefObject / AppDomain.CreateDomain)
are obsolete.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Benedikt Reinartz <filmor@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants